Conversation
…non-AdcpError (closes #562) PR #560 added context-echo on the AdcpError raise path. This test verifies the implicitly-wrapped path: a raw ValueError from a DecisioningPlatform method goes through _invoke_platform_method's except-Exception clause which wraps to AdcpError(INTERNAL_ERROR), which then projects through serve.py's catch + build_mcp_error_result with params=kwargs and echoes the request context. If a future refactor lets a non-AdcpError exception escape the dispatch wrap, this test fails first. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code-reviewer caught that the original assertion (INTERNAL_ERROR + context echoed) would pass even if the dispatcher wrap step were skipped. Adds details.caused_by.type == 'ValueError' assertion which is set only by _internal_error_details inside the wrap path. Other review nits: executor.shutdown(wait=True) for cleanup; trim PR-#560 history from docstring per CLAUDE.md; rename for symmetry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #562 (verification follow-up to #560).
Why
PR #560 added context-echo on the AdcpError raise path. The follow-up question was: does the dispatcher's exception-wrap path preserve that echo when a non-AdcpError exception escapes a platform method?
What I verified
`_invoke_platform_method` (`src/adcp/decisioning/dispatch.py`) catches every non-AdcpError in its `except Exception` clause and wraps to `AdcpError("INTERNAL_ERROR", recovery="terminal")`. Once wrapped, the AdcpError travels up to `serve.py`'s decisioning branch, which builds the structured envelope via `build_mcp_error_result(exc, params=kwargs)` — so context is echoed onto the wire.
This test pins the chain end-to-end:
If a future refactor lets a non-AdcpError exception escape the dispatch wrap (early-return path, middleware short-circuit, asgi_middleware that catches and re-raises differently), this test fails first — adopters lose correlation IDs across the boundary and we know about it.
Scope
Test-only change, no production code modified. The DecisioningPlatform path is fully covered. Raw `ADCPHandler` users (no DecisioningPlatform) currently see FastMCP's generic error on un-wrapped exceptions — separate concern; not addressed here.
Test plan
🤖 Generated with Claude Code